-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix refreshed dashboard losing time range #20858
Fix refreshed dashboard losing time range #20858
Conversation
💔 Build Failed |
Failed on:
and
The second failure showed a screenshot of the homepage. I feel like I've seen this flaky behavior before. Confirming... jenkins, test this. |
💔 Build Failed |
Okay failed again on other one passed so will file a flaky test issue for it (#20885) |
The test is failing because after saving the visualization it's redirecting to home page. I'm pretty sure I've seen this before and it's flaky. Tests also pass locally. Try again. jenkins, test this |
cc58e3b
to
2a21616
Compare
💔 Build Failed |
💔 Build Failed |
I now think this might be a legit issue. was able to repro locally by running more teste before the one that failed |
1d715e9
to
f467014
Compare
💔 Build Failed |
f467014
to
bb12fc6
Compare
💔 Build Failed |
Failed on jenkins, test this. |
@@ -69,6 +70,7 @@ uiModules | |||
require: '^kbnTopNav', | |||
link: ($scope, element, attributes, kbnTopNav) => { | |||
listenForUpdates($scope); | |||
updateGlobalStateWithTime($scope); | |||
|
|||
setTimefilterValues($scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call to setTimefilterValues
is no longer needed since updateGlobalStateWithTime
calls setTimefilterValues
@@ -69,6 +70,7 @@ uiModules | |||
require: '^kbnTopNav', | |||
link: ($scope, element, attributes, kbnTopNav) => { | |||
listenForUpdates($scope); | |||
updateGlobalStateWithTime($scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what fixed the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, global state wasn't initialized the first time on page load which only created a problem for dashboards stored with time. As soon as any interaction happened with the time picker, it'd update global state. Everything appeared to work correctly in the UI because that was set up correctly, it was just the URL that didn't match it.
💔 Build Failed |
Jenkins, test this - seemingly unrelated navigation error |
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
code review, verified changes fixes the linked issue
jenkins, test this |
0061438
to
65b28fb
Compare
This should also fix the other blocker, #20764 The problem was that if you visit a dashboard and set the time range to something, say Last 5 years, then go open a dashboard that had time stored with it, the dashboard would show the time saved with it, but the URL (which is also used for reporting) would contain the "last 5 years" time. |
💚 Build Succeeded |
💔 Build Failed |
Jenkins, test this - flaky test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, tested on Chrome Linux. Could reproduce the issue on master, can confirm this PR fixes it.
💔 Build Failed |
Failed on #19743 again jenkins, test this |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add test that would have caught the bug * Initialize global state with current time range * Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests * remove unneccessary extra call * Fix tests that failed due to globally added time in new tests * Update home page test to not care about any state.
* Add test that would have caught the bug * Initialize global state with current time range * Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests * remove unneccessary extra call * Fix tests that failed due to globally added time in new tests * Update home page test to not care about any state.
* Add test that would have caught the bug * Initialize global state with current time range * Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests * remove unneccessary extra call * Fix tests that failed due to globally added time in new tests * Update home page test to not care about any state.
* Add test that would have caught the bug * Initialize global state with current time range * Fix issue with failing tests - need to remove added "t" parameter to the url in the new tests * remove unneccessary extra call * Fix tests that failed due to globally added time in new tests * Update home page test to not care about any state.
Fixes #20827